Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Amazon Bedrock Provider #2016

Closed
wants to merge 26 commits into from
Closed

Conversation

jon-spaeth
Copy link
Contributor

@jon-spaeth jon-spaeth commented Jun 18, 2024

Adding an Amazon Bedrock provider package (@ai-sdk/amazon-bedrock) with the new ConverseApi

Copy link
Collaborator

@lgrammel lgrammel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I left a few comments.

@@ -0,0 +1 @@
# @ai-sdk/amazon-bedrock
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file can be removed, will be automatically created by changesets

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@@ -0,0 +1,72 @@
{
"name": "@ai-sdk/amazon-bedrock",
"version": "0.0.29",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set to 0.0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"dependencies": {
"@ai-sdk/provider": "0.0.10",
"@ai-sdk/provider-utils": "0.0.14",
"@aws-sdk/client-bedrock-runtime": "^3.598.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preference is usually to call the api directly using fetch, unless it's not easily possible because of e.g. auth. does the bedrock runtime have any limitations (e.g. does it work on edge)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a rest API for these calls so I could use fetch but I would have to convert the auth and I also would have to define all the types the library provides explicitly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auth can make this fairly complicated and is the reason why i used a similar approach for the vertex provider. fine to leave as is.

btw, would you mind documenting how auth works? (e.g. in a separate file, or in something similar to https://github.com/vercel/ai/blob/main/content/providers/01-ai-sdk-providers/11-google-vertex.mdx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added documentation with auth and model capabilities etc.

?.map(part => ({
toolCallType: 'function',
toolCallId: part.toolUse?.toolUseId ?? generateId(),
toolName: part.toolUse?.name ?? 'unknown',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when can this happen? can setting it to unknown lead to issues such as tools sharing the same name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory it shouldn't happen but the type of toolUseId and name are both string | undefined. Would it be better to leave it as an empty string?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strange, wonder when this could happen. how about tool-${generateId()}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Comment on lines 40 to 49
/**
Additional model parameters field paths to return in the response.
Converse returns the requested fields as a JSON Pointer object in
the additionalModelResponseFields field.

The following is example JSON for additionalModelResponseFieldPaths.

[ "/stop_sequence" ]
*/
additionalModelResponseFields?: string[];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are those captured in any way? or could this be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this and the system prompt settings properties

Comment on lines 28 to 29
// TODO: support image URL
bytes: part.image as Uint8Array,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. the anthropic provider supports this via image download

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added image download

Comment on lines 84 to 86
throw new Error(
`Unsupported message role '${role}'. Please use the system options on the provider instead.`,
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

support this instead of having a separate field. check out the anthropic provider for an example. this is a major incompatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied the way the anthropic provider did it

Comment on lines 79 to 81
throw new Error(
`Unsupported message role '${role}'. Please use the assistant role with toolUse content block instead.`,
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must have for tool roundtrips.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

Comment on lines 1 to 2
export * from '../bedrock-chat-language-model';
export * from '../bedrock-chat-settings';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file / package not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm not following, are you saying to remove this file completely, or just remove the bedrock-chat-settings export line?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the internal export completely. I only have it for openai bc it's needed by azure openai. otherwise I try to keep the api surface as small as possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Comment on lines 31 to 36
"./internal": {
"types": "./internal/dist/index.d.ts",
"import": "./internal/dist/index.mjs",
"module": "./internal/dist/index.mjs",
"require": "./internal/dist/index.js"
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the whole ./internal section?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Copy link

socket-security bot commented Jun 19, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@ai-sdk/[email protected] None 0 0 B
npm/[email protected] Transitive: environment, eval +7 6.41 MB m-radzikowski

View full report↗︎

"@aws-sdk/client-bedrock-runtime": "^3.598.0"
},
"devDependencies": {
"@smithy/types": "^3.1.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this to force the aws-sdk-client-mock to use this specific version because it gives a bunch of type errors if I don't

@lgrammel
Copy link
Collaborator

@jon-spaeth this is looking pretty good. small comments aside, some docs on how to do authentication and examples under examples/ai-core (so I can test it out) would be helpful

@jon-spaeth
Copy link
Contributor Author

@lgrammel I think I got everything you requested. Let me know if there is anything else! Thanks for the quick turnaround on this. It will be majorly helpful in my day to day.

@lgrammel
Copy link
Collaborator

Thanks this looks great! I'll take it from here. Superseeded by #2045

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants